Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[2.11] Enhancing the Install Demo Configuration Script with a clearer message #3490

Merged
merged 3 commits into from
Oct 7, 2023
Merged

[2.11] Enhancing the Install Demo Configuration Script with a clearer message #3490

merged 3 commits into from
Oct 7, 2023
+16 −4

Conversation

DarshitChanpura
Copy link
Member

Description

This changes remove the confusing log statement from 2.11 to unblock the build.

Issues Resolved

Testing

Manual testing: 
➜  opensearch-security ./tools/install_demo_configuration.sh -y -i -s
**************************************************************************
** This tool will be deprecated in the next major release of OpenSearch **
** https://github.com/opensearch-project/security/issues/1755           **
**************************************************************************
OpenSearch Security Demo Installer
 ** Warning: Do not use on production or public reachable systems **
Basedir: /Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT
OpenSearch install type: .tar.gz on
OpenSearch config dir: /Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/config
OpenSearch config file: /Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/config/opensearch.yml
OpenSearch bin dir: /Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/bin
OpenSearch plugins dir: /Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/plugins
OpenSearch lib dir: /Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/lib
Detected OpenSearch Version: x-content-3.0.0-SNAPSHOT
Detected OpenSearch Security Version: 3.0.0.0-SNAPSHOT

Unable to find custom admin password. Continuing with the default password for admin.
   ***************************************************
   ***   ADMIN PASSWORD SET TO: admin    ***
   ***************************************************
**************************************************************************
** This tool will be deprecated in the next major release of OpenSearch **
** https://github.com/opensearch-project/security/issues/1755           **
**************************************************************************
### Success
### Execute this script now on all your nodes and then start all nodes
### OpenSearch Security will be automatically initialized.
### If you like to change the runtime configuration
### change the files in ../../../config/opensearch-security and execute:
"/Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/plugins/opensearch-security/tools/securityadmin.sh" -cd "/Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/config/opensearch-security" -icl -key "/Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/config/kirk-key.pem" -cert "/Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/config/kirk.pem" -cacert "/Users/dchanp/Documents/amazon/testing-playground/test/binaries/opensearch-3.0.0-SNAPSHOT/config/root-ca.pem" -nhnv
### or run ./securityadmin_demo.sh
### To use the Security Plugin ConfigurationGUI
### To access your secured cluster open https://<hostname>:<HTTP port> and log in with admin/admin.
### (Ignore the SSL certificate warning because we installed self-signed demo certificates)

Check List

- [ ] New functionality includes testing
- [ ] New functionality has been documented

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@DarshitChanpura DarshitChanpura mentioned this pull request Oct 6, 2023
Closed
2 tasks
@peterzhuamazon peterzhuamazon mentioned this pull request Oct 6, 2023
Closed
71 tasks
@DarshitChanpura DarshitChanpura changed the title Replaces confusing log with a better error message [2.11] Replaces confusing log in install_demo_configuration script with a better error message Oct 6, 2023
peternied
peternied previously requested changes Oct 6, 2023
View reviewed changes
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its better, maybe I'm too close to the issue, but as it was it states the action(s) that can be taken to resolve the 'no password' scenario.

This new message doesn't indicate where it was looked for, or what action a user should take to get resolution. What didn't work about the old message for you?

@DarshitChanpura
Copy link
Member Author

@peternied The old message was confusing in the sense that if I spin up a docker instance and run it without passing initialAdminPassword the script would exit and not finish execution, however the docker cluster will continue to spin up and create an admin user with admin password as it picks that up from internal_users.yml file.

This PR avoids the confusion by letting users know that the password will be defaulted to admin which is what is eventually happening.

I have updated the log messages to be more detailed.

@DarshitChanpura DarshitChanpura mentioned this pull request Oct 6, 2023
Merged
1 task
@codecov
Copy link

codecov bot commented Oct 6, 2023 •
edited
Loading

Codecov Report

Merging #3490 (10bb68f) into 2.11 (ef1d27c) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               2.11    #3490      +/-   ##
============================================
- Coverage     64.69%   64.68%   -0.02%     
+ Complexity     3566     3565       -1     
============================================
  Files           267      267              
  Lines         19893    19893              
  Branches       3329     3329              
============================================
- Hits          12870    12868       -2     
- Misses         5383     5384       +1     
- Partials       1640     1641       +1

see 1 file with indirect coverage changes

cwperks
cwperks previously approved these changes Oct 6, 2023
View reviewed changes
RyanL1997
RyanL1997 previously approved these changes Oct 6, 2023
View reviewed changes
@DarshitChanpura DarshitChanpura changed the title [2.11] Replaces confusing log in install_demo_configuration script with a better error message [2.11] Replaces confusing log in install_demo_configuration script with a better message Oct 6, 2023
@DarshitChanpura DarshitChanpura changed the title [2.11] Replaces confusing log in install_demo_configuration script with a better message [2.11] Enhancing the Install Demo Configuration Script with a clearer message Oct 6, 2023
@DarshitChanpura DarshitChanpura dismissed stale reviews from RyanL1997 and cwperks via 95b4813 October 6, 2023 21:14
@peternied peternied dismissed their stale review October 6, 2023 21:50

Unblocking this review - has 2 approvals

@DarshitChanpura
Replaces confusing log with a better error message
c116fff 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Updates echo messages to be more detailed
86f8f16 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
Fixes missing double quotes
10bb68f 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura DarshitChanpura merged commit 919589c into opensearch-project:2.11 Oct 7, 2023
104 of 105 checks passed
@RyanL1997 RyanL1997 mentioned this pull request Oct 9, 2023
Closed
DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Oct 9, 2023
@DarshitChanpura
Revert "[2.11] Enhancing the Install Demo Configuration Script with a…
433eca6 
… clearer message (opensearch-project#3490)"

This reverts commit 919589c.
@DarshitChanpura DarshitChanpura mentioned this pull request Oct 9, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanL1997 RyanL1997 RyanL1997 approved these changes

@cwperks cwperks cwperks approved these changes

@cliu123 cliu123 Awaiting requested review from cliu123 cliu123 is a code owner

@davidlago davidlago Awaiting requested review from davidlago

@stephen-crawford stephen-crawford Awaiting requested review from stephen-crawford

@reta reta Awaiting requested review from reta reta is a code owner

@willyborankin willyborankin Awaiting requested review from willyborankin willyborankin is a code owner

@peternied peternied Awaiting requested review from peternied peternied is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DarshitChanpura @peternied @cwperks @RyanL1997